-
Notifications
You must be signed in to change notification settings - Fork 228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @najeal for your contribution 💯!
This looks really good to be the first draft 👍
I made a couple of suggestions, let me know what you think
pkg/preflight/checks.go
Outdated
return "ExistingFile" | ||
} | ||
|
||
type AvailablePathChecker struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems unused?
pkg/preflight/checks.go
Outdated
vmDir := filepath.Join(constants.VM_DIR, vm.GetUID().String()) | ||
kernelDir := filepath.Join(constants.KERNEL_DIR, kernelUID.String()) | ||
log.Println(vm.GetUID().String()) | ||
checks = append(checks, ExistingFileChecker{filePath: path.Join(vmDir, constants.METADATA)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check for the metadata and kernel files, they are gonna be created on the fly.
pkg/preflight/checks.go
Outdated
} | ||
vmDir := filepath.Join(constants.VM_DIR, vm.GetUID().String()) | ||
kernelDir := filepath.Join(constants.KERNEL_DIR, kernelUID.String()) | ||
log.Println(vm.GetUID().String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this?
pkg/preflight/checks.go
Outdated
for _, port := range vm.Spec.Network.Ports { | ||
checks = append(checks, PortOpenChecker{port: port.HostPort}) | ||
} | ||
return runChecks(checks, ignoredPreflightErrors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other things that could be added:
- make sure that the docker/containerd socket exists?
- this could be done by extending
runtime.Interface
with aPreflightChecker() preflight.Checker
method. when invoked, it would make sure docker / containerd sockets exist - here in this method you'd do
checks = append(checks, providers.Runtime.PreflightChecker())
- this could be done by extending
- make sure all the binaries listed in https://ignite.readthedocs.io/en/stable/dependencies.html are present in PATH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @najeal! 💯
Some small nits, otherwise LGTM 👍
Does need rebasing though before we can merge.
pkg/preflight/checkers/checks.go
Outdated
"strings" | ||
|
||
"k8s.io/apimachinery/pkg/util/sets" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this newline
checks = append(checks, ExistingFileChecker{filePath: "/dev/mapper/control"}) | ||
checks = append(checks, ExistingFileChecker{filePath: "/dev/net/tun"}) | ||
checks = append(checks, ExistingFileChecker{filePath: "/dev/kvm"}) | ||
checks = append(checks, providers.Runtime.PreflightChecker()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be an inline constructor instead of 4 allocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start operation preflight checks files exists and ports are available
…+ alloc properly the checkers array The package was not using the interface from the preflight package. We use it now and we deleted the interface in the checkers package. Multiple re-allocation was occuring when appending checkers to the array. Now, allocation is done at the beginning by calculating the total number of checkers we will store in the array.
04618ef
to
0256496
Compare
Done. |
This preflight would be a life saver, thank you @najeal !! It seems that we have some changes since last time, like
Small things left to be addressed. /cc @stealthybox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked to @chanwit and @stealthybox, this will be very valuable in v0.6.1.
Merging to unblock @chanwit.
LGTM, thanks @najeal 👍
This adds preflight checks before start operation.
Fixes #184